Use custom iterators to speed up inflow_ids and friends
#830
Conversation
|
@visr nice that you picked this up. Regarding the gap to close: this might be due to the dictionary to map from an edge to an index in the dense |
|
Also, |
| @@ -13,8 +13,8 @@ function allocation_graph_used_nodes!(p::Parameters, allocation_network_id::Int) | |||
| node_type = graph[node_id].type | |||
| if node_type in [:user, :basin] | |||
| push!(used_nodes, node_id) | |||
|
|
|||
| elseif length(inoutflow_ids(graph, node_id)) > 2 | |||
| elseif count(x -> true, inoutflow_ids(graph, node_id)) > 2 | |||
There was a problem hiding this comment.
Why is that preferable? I've assumed that length simply consumes unsized iterators, just like your code using count.
There was a problem hiding this comment.
No it's actually not defined, a MethodError.
This didn't show up in the profile so I think that's fast enough. Though the dict lookup to check if an edge is a flow edge did show up. This was for a model with only flow edges. @SouthEndMusic is this ok to merge for you? I think this fixes the issue enough to make a release, we can look after into more speedups. |
@Huite mentioned the performance of medium-sized models regressed recently, so I looked into it a bit, doing some profiling. I saw a lot of time was spent on finding the in- and outneighbors of flow. This changed quite a bit in #807, and whilst the API is a lot nicer, unfortunately there was also a performance regression. The profile showed allocations. This PR removes those allocations, leading to a 2x speedup for a test model, from 16s to 8s. Pre-#807 this ran in 3s however, so there is still a considerable gap to close. I don't really want to revert #807, though perhaps for flow connections we need another data structure, or some other way to speed up.
This PR changes from using a filtered array comprehension, which creates a
Vector{NodeID}, to a custom iteratorInNeighborswhich iterates over the filteredinneighbors. This avoids allocations but otherwise returns the same result.